Skip to content

Declare TwoWire functions as virtual #396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ArkadyGamza
Copy link
Contributor

Fix #94

To make alternative implementations of the TwoWire class (e.g. SoftwareWire for software I2C) work properly being passed to libraries that expect TwoWire type.

To make alternative implementations of the TwoWire class (e.g. SoftwareWire for software I2C) work properly being passed to libraries that expect TwoWire type.
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@ArkadyGamza
Copy link
Contributor Author

@aentinger could you please approve?

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@aentinger aentinger merged commit 5fb6220 into arduino:master May 25, 2021
@bxparks
Copy link
Contributor

bxparks commented May 25, 2021

This causes flash memory to increase by ~650 bytes, and static RAM to increase by 42 bytes on AVR processors. If SoftwareWire does not require every method to be virtual, a more surgical approach might be preferable.

Tested on:

  • Two sketches:
    • one talking to just a DS3231 RTC over I2C
    • one talking to a DS3231 and an SSD1306 OLED display over I2C
  • Arduino Nano, ATmega328, Arduino AVR Core 1.8.3
  • SparkFun Pro Micro, ATmega32U4, SparkFun AVR Core 1.1.13

@facchinm
Copy link
Member

This ⬆️ is "a bit" dangerous, and it could be prevented by adding the CI for code size on this repo too. @per1234 do you have some time to create a PR? Even without external libraries, just the bundled ones.
I'm in favour of reverting this BTW, the flash increase is too high and could prevent a lot of existing sketches from compiling.

@aentinger
Copy link
Contributor

Good by me, I'll revert it.

@per1234
Copy link
Contributor

per1234 commented May 26, 2021

@per1234 do you have some time to create a PR? Even without external libraries, just the bundled ones.

You got it! #413
I set it up for only the bundled libraries. I'm happy to expand it on request.

We could increase the core library coverage without introducing too much in the way of external dependencies by adding in compilation of the built-in examples, as was done in the sketch compilation workflows for the megaAVR and SAMD platforms .

@bxparks
Copy link
Contributor

bxparks commented May 26, 2021

Even though this PR was reverted, I want to give folks like @ArkadyGamza, @ahnolds (#331) and @SRGDamia1 (#94) some hope about using SoftwareWire. Recently, I have had some success using compile-time polymorphism (using C++ templates) instead of runtime polymorphism (using the virtual dispatch and inheritance). I think the technique can be used for SoftwareWire.

So instead of using TwoWire as the polymorphic base class, it might be possible to rewrite the client code to use the TwoWire and SoftwareWire objects directly through the C++ template mechanism, without going through the dynamic virtual dispatch, like this:

template <typename WIRE>
class MyClass {
  public:
    explicit MyClass(WIRE& wire) : mWire(wire) {}

    void sendInfo() {
      mWire.beginTransmission(address);
      mWire.write(...);
      mWire.endTransmission();
      ...
    }

  private:
    WIRE& mWire;
};

// Use <Wire.h>
MyClass<Wire> instanceUsingWire(Wire);

// Use <SoftwareWire.h>
SoftwareWire softwareWire(SDA, SCL);
MyClass<SoftwareWire> instanceUsingSoftwareWire(softwareWire);

I did a quick test of this with one of my programs, and got it working with both TwoWire and SoftwareWire. The flash memory increases by about ~250 bytes, due to the extra indirection through the mWire member variable, but that's far smaller than the ~650 byte increase using virtual dispatch.

One final comment about SoftwareWire: It contains a virtual destructor ~SoftwareWire() (https://github.com/Testato/SoftwareWire/blob/master/SoftwareWire.h) which is probably unnecessary because this object is unlikely to be created on the heap and deleted through its pointer. I have found that the simple declaration of a virtual destructor, even if it is never used, increases flash memory consumption on AVR processors by ~600 bytes, because it pulls in the free() and malloc() functions. I am actually convinced that it is incorrect to provide a virtual destructor in SoftwareWire, because none of the parent classes (i.e. TwoWire, Stream, Print) have a virtual destructor, so SoftwareWire cannot be deleted polymorphically at all.

@bperrybap
Copy link

IMO, doing something like this (making all functions virtual) is not the solution to the issue.
It comes with a cost and can never fully solve the issue for libraries that have certain things hardcoded.

In the bigger picture I think how to update/modify things to better support multiple Wire/i2c libraries, multiple Wire/i2c objects for multiple i2c buses needs quite a bit of thought and discussion. Seems like something for a good discussion on the mailing list?

The issue I see for this type of solution, is that it is attempting to try to resolve an issue that is outside the library being updated/modified.
i.e. the issue is not in the "Wire" object, its class, its class name, or even how functions are declared but rather in the libraries that are hardcoded to use the Wire object or the TwoWire class.
Yes, it is a real issue for some libraries that use "Wire" services, but the issue is in those libraries, not the "Wire" libraries.
It needs to be solved in those other libraries since it can never be fully resolved by making changes to the Wire library and its class.
Even if you jump through many hoops in an attempt to try to make something "work" for those other hardcoded libraries, there will always be issues until those other libraries are fixed to no longer hard code the Wire library object name or its class.
The best you can do is kind of sort of make it better for some use cases, but there will still be many common use cases that do not work.

Also, does everyone understand that virtual functions are always linked in regardless of whether they are used or not?
It isn't like "normal" functions that can removed though a combination of the compiler and linker when they are not referenced.
The linker has no visibility to be able to determine if virtual functions are ever referenced.
There used to be a way to patch the class vtable entries for the object so the linker could be told that this virtual function will never be used and can be removed from the final linked image, but that was removed from more recent versions of gcc tools.
So as of today, if you declare a function virtual it will always be linked in.
So depending on how this is handled, and the particular sketch, you can end up with lots of dead code being linked in.

IMO, the proper way to fix this is to update the libraries that use Wire services, after all it is their issue.
They can either

  • move all their code into the class definition in the .h rather than put it in a .cpp file and depend on an object name Wire
    Library will not include <Wire.h> (it will be included by sketch) and then depend on an object named Wire
    This is an easy update and is the highest bang for the buck as it covers most of the simple and most common use cases and allows SoftwareWire to work as long as either the library or the user declares the object named Wire.
    (this is what the hd44780 library does)
    Note this methodology only allows SoftwareWire to substitute for Wire, it does not allow both to coexist and be used by a sketch.

  • similar to above but use a template and polymorphism in the library (not the Wire library)
    This will allow the library to work with any "Wire" library regardless of class name or object name.
    IMO, this is the proper way to handle this as it will work for any class name and even allows multiple Wire libraries and objects to be used concurrently by the sketch.
    i.e. you could use Wire and Software at the same time or support platforms that have more than one i2c bus that may have different object names for each. i.e. say Wire vs Wire2 etc...
    A few Arduino libraries currently do this. I know of at least one.
    (The hd44780 library will be updated to use this methodology in the future)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Wire] make TwoWire all class functions virtual
7 participants